fix: range-check Calibur key settings so expiration cannot set the isAdmin bit - #222
Merged
Conversation
…Admin bit packKeySettings shifted expiration into bits 160+ without validating it fits the contract's uint40 field. A millisecond timestamp (>= 2^40) overflowed exactly into bit 200 — the isAdmin flag — registering the key as admin and bypassing the explicit guardrail in createRegisterKeyMetaTransactions. Both expiration and hook are now range-checked with a hint about seconds vs milliseconds. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
📝 WalkthroughWalkthrough
ChangesCalibur key settings validation
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/calibur/packKeySettings.test.js`:
- Around line 18-21: Update the test case “rejects a milliseconds timestamp
instead of leaking into isAdmin” to assert that the thrown RangeError message
includes the actionable guidance indicating the expiration value must use
seconds rather than milliseconds. Keep the existing RangeError assertion and
verify the message content on the same thrown error.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 006b19ef-8ee1-4130-84b8-0ba01dc77960
📒 Files selected for processing (2)
src/account/Calibur/Calibur7702Account.tstest/calibur/packKeySettings.test.js
Comment on lines
+18
to
+21
| test('rejects a milliseconds timestamp instead of leaking into isAdmin', () => { | ||
| const ms = 1786000000000n; // >= 2^40: would set bit 200 (isAdmin) | ||
| expect(() => Calibur.packKeySettings({ expiration: ms })).toThrow(RangeError); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the actionable expiration error hint.
This test only checks RangeError; it would still pass if the promised “milliseconds instead of seconds” guidance were removed.
Suggested assertion
- expect(() => Calibur.packKeySettings({ expiration: ms })).toThrow(RangeError);
+ expect(() => Calibur.packKeySettings({ expiration: ms })).toThrow(
+ /milliseconds instead of seconds/i,
+ );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| test('rejects a milliseconds timestamp instead of leaking into isAdmin', () => { | |
| const ms = 1786000000000n; // >= 2^40: would set bit 200 (isAdmin) | |
| expect(() => Calibur.packKeySettings({ expiration: ms })).toThrow(RangeError); | |
| }); | |
| test('rejects a milliseconds timestamp instead of leaking into isAdmin', () => { | |
| const ms = 1786000000000n; // >= 2^40: would set bit 200 (isAdmin) | |
| expect(() => Calibur.packKeySettings({ expiration: ms })).toThrow( | |
| /milliseconds instead of seconds/i, | |
| ); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/calibur/packKeySettings.test.js` around lines 18 - 21, Update the test
case “rejects a milliseconds timestamp instead of leaking into isAdmin” to
assert that the thrown RangeError message includes the actionable guidance
indicating the expiration value must use seconds rather than milliseconds. Keep
the existing RangeError assertion and verify the message content on the same
thrown error.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
packKeySettingsshiftedexpirationinto bits 160+ without validating it fits the contract's uint40 field. A millisecond timestamp (≥ 2^40) overflowed exactly into bit 200 — theisAdminflag — registering the key as admin and bypassing the explicit guardrail increateRegisterKeyMetaTransactions.Both
expirationandhookare now range-checked, with an error hint about seconds vs milliseconds since a ms timestamp is the most likely way to hit this.Tests added in
test/calibur/packKeySettings.test.js; full offline suite passes.Summary by CodeRabbit
Bug Fixes
Tests